fix(plugins): let callbacks opt out of short-circuit via shortCircuit: false#343
fix(plugins): let callbacks opt out of short-circuit via shortCircuit: false#343tSte wants to merge 3 commits into
Conversation
…t: false`
## Problem
`PluginManager.runCallbacks` short-circuits the plugin chain as soon as
a callback returns a non-`undefined` value, which silently disables
every plugin registered after a plugin that legitimately transforms a
value (localization, redaction, content filtering). Logging, metrics
and other observer-style plugins never get a chance to run.
## Change
Adds an opt-out: if a callback returns an object whose literal
`shortCircuit` field is `false`, the flag is stripped, the remaining
fields become the new *current* value, and the chain continues. The
next plugin receives that value spliced into the right input field
(`result` / `llmResponse` / `event` / `userMessage` / `tools`), so
transforms genuinely compose.
Any other non-`undefined` return preserves the legacy early-exit
behavior, so existing plugins are unaffected.
`BasePlugin` return types for `Content` / `Event` / `LlmResponse` /
tools-map are widened with \`& { shortCircuit?: boolean }\` so overrides
type-check under strict mode without casts. \`Record<string, unknown>\`-
typed callbacks need no change.
Closes google#342
- Rename ShortCircuit to ShortCircuitControl and make `shortCircuit` optional, so a plugin returning a plain Content/Event/LlmResponse still satisfies the callback return type (the required field was a breaking change for existing plugins). - runCallbacks defaults a missing flag to true and strips `shortCircuit` from the value on every path, including the short-circuit path. - afterToolCallback return type carries ShortCircuitControl, matching the other value-returning callbacks it is threaded alongside.
AmaadMartin
left a comment
There was a problem hiding this comment.
Here is a review of the PR. The overall idea of allowing plugins to opt-out of short-circuiting makes sense, but the implementation is inconsistent and has some issues that need to be addressed before merging. Specifically, many callbacks were updated in base_plugin.ts but are not correctly wired for chaining in plugin_manager.ts.
| private async runCallbacks( | ||
| plugins: Set<BasePlugin>, | ||
| callback: (plugin: BasePlugin) => Promise<unknown>, | ||
| callback: (plugin: BasePlugin, latest: unknown) => Promise<unknown>, |
There was a problem hiding this comment.
This new signature (accepting latest value for chaining) is only utilized for a subset of callbacks in plugin_manager.ts (e.g., onUserMessageCallback, onEventCallback, beforeToolSelection, afterToolCallback, afterModelCallback).
Other callbacks that had their types updated in base_plugin.ts (like beforeRunCallback, beforeAgentCallback, afterAgentCallback, beforeModelCallback, onModelErrorCallback) were not updated in plugin_manager.ts to pass the latest value. This means they will not actually chain/transform values correctly (they will run, but won't see previous plugins' changes).
| agent: BaseAgent; | ||
| callbackContext: Context; | ||
| }): Promise<Content | undefined> { | ||
| }): Promise<(Content & ShortCircuitControl) | undefined> { |
There was a problem hiding this comment.
afterAgentCallback return type is updated to support ShortCircuitControl, but the method signature doesn't receive the agent's result in its parameters. Without receiving the result, it is impossible for a plugin to transform it. We should add result to the parameters and update plugin_manager.ts to pipe it.
| callbackContext: Context; | ||
| llmRequest: LlmRequest; | ||
| }): Promise<LlmResponse | undefined> { | ||
| }): Promise<(LlmResponse & ShortCircuitControl) | undefined> { |
There was a problem hiding this comment.
Why was beforeModelCallback updated to support ShortCircuitControl here, but beforeToolCallback was not? Both are "before" callbacks that can return a value to skip the operation. They should be treated consistently. (Note: if they are strictly for vetoing, they probably shouldn't support chaining/ShortCircuitControl at all, as they don't have parameters to receive the chained value).
| toolContext: Context; | ||
| result: Record<string, unknown>; | ||
| }): Promise<Record<string, unknown> | undefined> { | ||
| }): Promise<(Record<string, unknown> & ShortCircuitControl) | undefined> { |
There was a problem hiding this comment.
Note: Immediately following this method (starting at line 418), there is a pre-existing unclosed comment block (/** without closing */). This causes the compiler to treat the entire block up to line 441 as a comment, silently commenting out a duplicate/broken method definition in between. We should clean this up while we are modifying this file.
Addresses review feedback on google#343. ShortCircuitControl was added to many callback return types in base_plugin.ts but only the callbacks that receive and transform a value are wired for chaining in plugin_manager.ts (onUserMessage, onEvent, beforeToolSelection, afterTool, afterModel). The remaining callbacks are veto-style: they have no parameter to receive the previous plugin's value, so `shortCircuit: false` could never chain and was type-only. - Remove ShortCircuitControl from beforeRunCallback, beforeAgentCallback, afterAgentCallback, beforeModelCallback, and onModelErrorCallback. This also makes the before* callbacks consistent (beforeToolCallback was never changed). afterAgentCallback in particular cannot transform a result it does not receive. - Remove a pre-existing broken/unclosed comment block after afterToolCallback that silently commented out the onToolErrorCallback doc and left a stray method fragment in the source.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Problem:
PluginManager.runCallbacksshort-circuits the plugin chain as soon as a callback returns a non-undefinedvalue. A plugin that legitimately transforms a value (localization rewritingLlmResponse, redaction altering a tool result, content filtering) silently disables every plugin registered after it for that callback — logging, metrics, tracing, safety auditing, etc. The failure is silent (only alogger.debugline is emitted) and there is currently no way for a plugin to express "I transformed this, keep going."The
BasePlugindocstring already documents the intended pipeline semantic ("The modifications will be visible and passed to the next callback in the chain"), but the implementation inPluginManager.runCallbacksdoes not match it.Solution: Add an opt-out path. If a callback returns an object whose literal
shortCircuitfield isfalse, the flag is stripped, the remaining fields become the new current value, and the chain continues. The next plugin receives that value spliced into the appropriate input field (result/llmResponse/event/userMessage/tools), so transforms genuinely compose. Any other non-undefinedreturn preserves the legacy early-exit behavior, so existing plugins are unaffected.Example:
BasePluginreturn types forContent/Event/LlmResponse/ tools-map are widened with& { shortCircuit?: boolean }so overrides type-check under strict mode without casts.Record<string, unknown>-typed callbacks (beforeTool/afterTool/onToolError) need no widening.Testing Plan
Unit Tests:
plugin_manager_test.tscases pass unchanged (legacy short-circuit semantics preserved).{ shortCircuit: false, ...x }keeps plugin2 running and strips the flag from the final result.resultparam (pipeline).Manual End-to-End (E2E) Tests:
Verified against a real voice-agent setup with a
ResponseMessagePlugin(localizesresult.message) registered before aLoggingPlugin(logs the final tool result). Before the fix, the logging plugin was never invoked for any tool that produced a localized message. After the fix, the logging plugin runs and logs the translated message, confirming both the opt-out and the pipeline plumbing.Checklist
Additional context
The change conflates two distinct intents that the current short-circuit logic merges:
beforeModelCallbackreturning a cachedLlmResponseto skip the model call. Early exit is correct.afterModelCallbacktranslating the response. Early exit is harmful; the next plugin should see the transformed value.The
shortCircuit: falseopt-out lets callers express the second intent without changing the default behavior of the first. All short-circuiting callbacks are affected:onUserMessageCallback,beforeRunCallback,onEventCallback,beforeAgentCallback,afterAgentCallback,beforeToolSelection,beforeToolCallback,afterToolCallback,onModelErrorCallback,beforeModelCallback,afterModelCallback,onToolErrorCallback.